fix: terminal input and cleanup by joshuapare · Pull Request #47 · omniviewdev/omniview

joshuapare · 2026-03-22T14:51:29Z

Summary

Base64-encode terminal input before calling WriteSession — Wails v3 serializes []byte as base64 over JSON, but the frontend was sending raw keystrokes, causing every keystroke to fail with illegal base64 data at input byte 0
Reduce terminal debug log noise — remove raw ANSI buffer dumps, per-keystroke input logs, and debug breadcrumbs
Centralize signal handling — single Manager-level goroutine forwards host-process signals to all child terminals instead of one listener per session competing for signals
Fix ListSessions lock — use RLock instead of Lock for read-only operation
Remove dead code — unused cleanPTYOutput and listenOnOut functions

Test plan

Open a local terminal in Omniview — verify keystrokes are accepted and output renders
Open multiple terminals simultaneously — verify signals don't interfere
Verify plugin exec sessions (e.g. Kubernetes) also accept input
Check logs are clean without raw ANSI dumps or per-keystroke spam

Summary by CodeRabbit

Refactor
- Enhanced terminal session management with optimized signal handling and command tracking
- Updated terminal input data encoding for consistency

Wails v3 serializes []byte parameters as base64 over JSON. The frontend was sending raw keystrokes which failed with "illegal base64 data".

- Remove raw ANSI buffer dump, log buffer size instead - Remove per-keystroke "got input" log from exec mux - Remove "past lock" breadcrumb log - Log session count instead of full session objects in ListSessions - Log only command and tty in StartSession instead of full opts/context

- Centralize host-process signal forwarding into a single Manager-level goroutine instead of one per terminal session. Prevents signal delivery contention with multiple terminals open. - Use RLock instead of Lock in ListSessions (read-only operation). - Remove unused cleanPTYOutput function and listenOnOut function.

coderabbitai · 2026-03-22T14:51:47Z

Warning

Rate limit exceeded

@joshuapare has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d3bd04f1-2d11-4acb-b5cd-1e43e2f1af94

📥 Commits

Reviewing files that changed from the base of the PR and between a3fd899 and ceca022.

📒 Files selected for processing (2)

backend/pkg/plugin/exec/controller.go
backend/pkg/terminal/manager.go

📝 Walkthrough

Walkthrough

Backend refactoring shifts terminal session lifecycle management from external cancel channels to context-based control, introduces manager-wide signal forwarding for tracked commands, and removes debug logging. Frontend adds Base64 encoding for keystroke data before transmission.

Changes

Cohort / File(s)	Summary
Control Flow Context Refactoring `backend/pkg/plugin/exec/controller.go`, `backend/pkg/terminal/manager.go`, `backend/pkg/terminal/manager_windows.go`	Replaces per-session external cancellation via `listenOnOut()` goroutine with context-based lifecycle management. `terminal.Manager` now accepts `context.Context`, stores it, and manages session termination through context cancellation. Signal forwarding refactored from per-session handlers to manager-wide iteration over tracked commands.
Session Tracking & Signal Handling `backend/pkg/terminal/manager.go`	Introduces `cmds map[string]*exec.Cmd` to track active session commands. Implements `forwardSignals()` goroutine that broadcasts `SIGTERM/SIGINT/SIGQUIT` to all tracked commands. Replaces per-session `handleSignals()` with `handleSessionClose()` that detects session context cancellation and emits `StreamSignalClose`. Cleanup logic removes command entries in `terminateSessionLocked()`.
Debugging & Logging Adjustments `backend/pkg/plugin/exec/controller.go`, `backend/pkg/terminal/manager.go`	Removes debug log for incoming plugin stream inputs (`"got input"`). Updates logging in `StartSession` and `AttachSession` to include command/TTY and session/buffer metadata. Changes `ListSessions` from `Lock` to `RLock` and logs session count instead of individual sessions.
UI Keystroke Encoding `ui/providers/BottomDrawer/containers/Terminal.tsx`	Encodes keystroke payload with Base64 before calling `ExecClient.WriteSession()`, changing from raw `data` to `Base64.encode(data)`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 With whiskers twitching, I spy the change,
No more loose threads in signals' range!
Context commands, tracked so neat,
Base64 keystrokes, Base-encoded and sweet!
Manager goroutines now forward with care,
A cleanup so proper—quite rare! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: terminal input and cleanup' accurately reflects the main changes: Base64-encoding terminal input, refactoring signal handling, and removing debug noise.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/terminal-input-and-cleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/exec/controller.go`:
- Around line 120-122: The controller currently initializes
terminal.NewManager(...) inside runLocalMux and assigns c.terminalManager there,
causing unsynchronized reads (nil/datarace) from methods like CreateSession,
ListSessions, GetSession, AttachSession, DetachSession, WriteSession,
CloseSession, and ResizeSession; instead, call terminal.NewManager(c.ctx,
c.logger) in ServiceStartup, assign the resulting manager to c.terminalManager
before spawning the goroutine, and pass the returned inMux, outMux, resizeMux
into runLocalMux as parameters so runLocalMux uses those channels but does not
perform the manager assignment (move creation/assignment out of
controller.runLocalMux into ServiceStartup and change runLocalMux signature to
accept inMux, outMux, resizeMux).

In `@backend/pkg/terminal/manager.go`:
- Around line 35-36: The session contexts are rooted in context.Background() so
shutting down m.ctx doesn't cancel active PTYs; update the code that creates
each session (the spot that currently uses context.Background() — referenced
around the session/spawn logic and line creating the session context) to derive
from the manager context (use m.ctx or context.WithCancel(m.ctx)) instead of
context.Background(), ensuring forwardSignals() exiting on m.ctx.Done() will
cancel sessions and trigger handleSessionClose; also verify any goroutines that
run per-session accept that derived context and propagate it to the PTY
lifecycle and handleSessionClose invocation.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7dd2f5be-f43a-4708-abc5-2c81a17a00f5

📥 Commits

Reviewing files that changed from the base of the PR and between dd2bd55 and a3fd899.

📒 Files selected for processing (4)

backend/pkg/plugin/exec/controller.go
backend/pkg/terminal/manager.go
backend/pkg/terminal/manager_windows.go
ui/providers/BottomDrawer/containers/Terminal.tsx

- Move terminal.NewManager() from runLocalMux goroutine into ServiceStartup so c.terminalManager is initialized before any goroutine can access it. - Derive session contexts from m.ctx instead of context.Background() so manager shutdown cascades to all active PTY sessions.

joshuapare added 3 commits March 22, 2026 09:45

fix: base64-encode terminal input before WriteSession

ff222ba

Wails v3 serializes []byte parameters as base64 over JSON. The frontend was sending raw keystrokes which failed with "illegal base64 data".

coderabbitai Bot reviewed Mar 22, 2026

View reviewed changes

Comment thread backend/pkg/plugin/exec/controller.go Outdated

Comment thread backend/pkg/terminal/manager.go

joshuapare merged commit dd6cd05 into main Mar 22, 2026
8 checks passed

coderabbitai Bot mentioned this pull request Mar 28, 2026

fix: render buffered output on terminal session attach #51

Merged

1 task

Navigation Menu

Uh oh!

fix: terminal input and cleanup - #47

fix: terminal input and cleanup#47
joshuapare merged 4 commits into
mainfrom
fix/terminal-input-and-cleanup

joshuapare commented Mar 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 22, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

joshuapare commented Mar 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

joshuapare commented Mar 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 22, 2026 •

edited

Loading